What does this do? What is python-gear and why does it say we need to change it?

That's a default variable value (a random recent koji package with python code in it), so that you can run ansible-playbook tests.yml without studying and adding any variables on the command line, and still see it doing something.

ok

It's 2018 and you've just wrote a Python 2 only script. Every time you do that, some little part in me dies. We are not going to maintain a Legacy Python only script in our repo. It's already tedious to keep in mind that our code here needs to be Python 2 compatible, this is just making it worse. I suppose this code should be part of taskotron, not our repo, but I'll get to this in a different section.

That's because I'm not familiar with Python 3, even in 2018 :-/ This code is a semi-temporary file that I've already copied to several tasks and I'm definitely not happy about it. It's there because:
a) invoking koji download-build command directly is not sufficient for this task (you need build.logs)
b) you can't interact with python directly in ansible, so you need a wrapper that accepts cmdline arguments and converts them into python calls which use our koji_directive helper library

In the future, we'd like to have a native ansible module that exposes all important input arguments, so that you can call it directly and don't need to have a wrapper.

Of course, if you decide to convert this to python23 in the meantime (or even a better idea), I have no issue with it (quite the opposite).

Is this how you run the checks in production now instead of the runtask command? Will this command give the yaml output that we can parse as we did in our integration tests? We want to make sure the tests work in Taskotron not, that some ansible playbook works.

Basically, yes. We set up more variables. Here's an example:
http://taskotron-dev.fedoraproject.org/artifacts/all/6ea80e0c-0756-11e8-920b-525400ee7c53/task_output/tests.yml/taskotron/ansible_vars.json.gz

The ResultYAML file will be stored in {{artifacts}}/taskotron/results.yaml, which for local execution defaults to ./artifacts/taskotron/results.yaml. That's exactly the file we will then send to ResultsDB.

This is as close as possible to actual taskotron execution, so verifying this is useful, I believe. Of course it's not the same as running through the whole taskotron stack.

Our integration tests now read the results.yaml file.

Did we just get rid of all the other architectures?

Again, that's a default variable value, for local execution when developing/experimenting. It is expected that you override this (and taskotron stack will do that for you, with the list of arches we want to execute on).

ok

Can we invoke python2 here directly instead of adding a shebang and executable flag? Would that break something?

Shouldn't break anything, I believe. I'm simply used to the current approach, that's why I wrote it that way.

Instead of our home brewed script, maybe a takotorn_download_rpms --with_logs command should exist and be maintained upstream and used here. How are the other checks doing this?

It could, yes. As replied above, a more preferable approach is a native Ansible module. But right now, this is what I have ready.

What is the actual reason to disable the tests? If I change the integration tests to run the ansible playbook in mock, would that work?

On it.

OK, there is a way, I have a very rough code ready, testing it.

I have no idea :-) The reason I disabled it is because I don't have time ATM to implement running the test suite in docker/mock/something else, and I hoped you'll either don't care that much (since with ansible playbook, taskotron runner is now a very thin wrapper around all of this), or implement it yourself. Or I might get to it, eventually. Just not immediately.

pushed, let's see what Travis says

What Fedora version is this running on in production?

The approach hasn't changed, we pick the image to run on based on the item tested. So in this case we parse NVR and use disttag to decide which image to spin up.

This is bad design, it worked before and now it requires this extra step (getting a system where this runs). Once I setup a mock based workaround for this, I'll provide a way to run it not only in the integration tests, but also manually.

Agreed. I proposed some ideas how to mitigate this at least for local execution in the PR description.

Added mock section bellow.

There was a --debug switch to runtask. How do I turn on debug logging now?

Currently, when you use runtask --debug, we do two things:

1. Switch taskotron runner to debug mode
2. Append -vv to ansible-playbook invocation.

Since the task no longer runs on the same machine and in the same process as the runner, our setting no longer applies to libtaskotron libraries being used inside the task. However, you can set up your own ansible variable and if it is present, you can switch your task to debug output (for libtaskotron libraries, simple use logging.getLogger('libtaskotron').setLevel(logging.DEBUG)). Even better, we can expose an ansible variable taskotron_debug: bool and you can react on that. Which means the task will show debug output even when run through runtask and there will be a standard way to toggle this for all tasks (that support that).

That being said, all of this might be of limited use and I'm actually not sure it's needed. Since the task is now run through ansible, you now longer see its standard output (at least not in realtime). Instead, you're required to create {{artifacts}}/test.log file containing log output of your task. That can be always put to debug level, if you want. Or you can create test.log with standard level output, and test.log.debug with debug level output, every time. It's your preference. We will always archive whole {{artifacts}} dir. Still, if you prefer a boolean toggle, I can expose it easily.

keep it how it is, thanks for explanation

Thanks. Actually, I messed up this a little when converting from koji download-build to download_rpms.py, so I'll fix it in a following commit.

I'm not sure if this is the right way of doing this, but I found not way to "kill all processes from mock".

Note that sometimes I had problem when I tried to Ctrl+C the tests, that it only killed the mock process and the entire pytest thing detached from my stdin, so it was running in background and I could not Ctrl+C it any more. This is very inconvenient, so I recommend running he integration tests with --maxfail=1 when running interactively.

This ends up with positive exit code when the check FAIL. So I didn't check the result. However there is failure (of the check) and failure (of the entire circus) and we should check the exit status and try to guess (at least if it's negative, it was killed and we should raise KeybordInterupt or something like that).

Added, it's either 0 or 2.

To clarify: the SI spec says the playbook exit code reflects the result of the check - 0 means PASS, non-zero means FAIL. It doesn't know the concept of generic tests. To stay close to the SI spec, we opted (at least for now) to exiting with 0 if all tests pass and non-zero if at least one check fails in our generic tasks. Unfortunately that prevents us from distinguishing a failed test from a crashed test. So, as long there is {{artifacts}}/taskotron/results.yml present, we consider the test to have finished fine. This is something we should discuss with SI spec authors in the future.

The log contains all the previous runs, we need to purge it before each test.

testing this

I might be missing something, but test.log should be overwritten in the Download RPMs from Koji ansible step. A possible approach is also to use a different artifacts directory for each integration test suite invocation.

It contained other stuff. Now I need to preserve each artifacts dir for each nevr, but that's fine. it will also make it easier when debugging test suite failures.

The testsuite actually only worked because it got preserved. So this was clearly a bug in the design of the tests (previously, each runtask had a different artifact path, now they shared it).

Pushed. Also, this may allow us to add a --fake switch that will not run the checks but use the stuff from the artifacts directories (e.g. if you are only changing the tests but not the implementation).

Added that as well.

Not sure why this gets removed.
An own directory gets created for each nevr only during integration tests so far. When running ansible-playbook ... command the results get saved to artifacts.

Are you overriding artifacts variable on the command line?
ansible-playbook ... -e artifacts=path

Nevermind, maybe I misunderstood the question. Either way, I also believe it's better to keep this in .gitignore. When somebody runs the playbook with default vars, that's where the artifacts get created.

I am not overriding artifacts, I just took README and tried running as it says :)
I will add it back.

Besides, while test.log gets overridden with each subsequent run, output.log keeps previous results (also with default vars).

ok, get it back, my bad, I haven't considered direct runs.

@irushchyshyn When running it locally, it's your duty to clear artifacts dir (or not). The task can't know your intended use case. Of course, you can e.g. generate random artifacts dir paths inside the playbook (the same way it generates workdirs), in case the variable is not set externally. It's your task, you can do whatever you prefer for local runs :) Local runs are mostly for development.

@kparal thanks for explanation.